The culling mechanism sometime crashes when going outside/ on the border of the game plane . Non the less, I please you to test it under other cases and report the bugs if you find it.

I confirm that there's a big FPS gain when starting the tile culling, especially in big maps. It does crash when reaching the borders of the level, though.

Another issue seems to be that the minimap does not really work properly, probably because the tiles it should render are culled.

Cool to see progress there! I do hope you'll be able to fix the bugs noticed by @akien-mga :)

Bertram25 : OK , may I set the limits for camera position ? Actually see my Issue >> Eliminating black space << please ... :)
What do you think about bringing the old mini-map ? It looked quite old - school and it was better than rendered mini-map IMHO . ( Which now suffers from custom tile culling ) ....

@ALL ; Have you noticed this crash happens only for edges at X = 0 or Y =0 leaving the other pair intact ... seems this bug would be quickly found ....

I've just tested and confirm the game exists when map borders are reached. Moreover, during game, many tiles are not displayed.

Ok , I fixed all that , the only problem lurking is the minimap presentation layer . Would you like the old one back ?

im a fan of the old one...

I confirm that the crashes at the border are gone. I didn't see the issue in @hwoarangmy's screenshot so far, on the other hand when you move very quickly with the arrows / WASD, you can see some culled tiles in the direction where you're moving.

I tried to even enlarge the area with myArray.zoom(1.2532); but it helps only a little.
Could it be fixed ? Maybe we could arrange lower maximal speed for the camera ?
Problem is inside Ogre3d , that it does not link / unlink objects in SceneNodes' tree instantly .
For me it's less annoying than the whole board hoovering in black space ...

I still expect some help with making it automaticly start startTileCulling and stopTIleCulling at the beginning and at the end from the other part of the team .

@OpenDungeons/developers Tom merged all commits into one as the single commits were not really meaningful taken alone. It's ready for review I believe :)

Cool! I'll have a look asap. (Busy as hell, but still around ;])

The code still has a lot of useless spaces and newlines all over the place, could you fix that?
Also, the headers copyright are stating 2014.

Btw, why are you using extern values of Units and precision digits? Couldn't this be avoided by setting a correct value an set it in a proper common header?

@tomluchowski That's better, indeed. However, you have not removed the now useless headers from the cameramanager and not answered some of my questions in code.

Concerning SlopWalk, could you please explain what the values in the arrays are supposed to be ? I couldn't guess from reading the code.

Note that when you will have to cull entities on tile, you can easily do it by using Tile::mEntitiesInTile (on client side, creatures and most of the entities are added to mEntitiesInTile even the moving ones).

How about now ?

I will review the whole PR tomorrow but I guess it should be good.
I will also try it.

I've tried but there is still the bug with the minimap. However, once it is fixed, it is mergeable for me.

@tomluchowski For my information, I've seen you use 4 Ogre rays to know the tiles the player can see (I guess by checking the 4 corners of the screen). Once there, I would have expected a simple linear extrapolation to know all the tiles within vision. Is the algorithm you used better in some way ? I must admit I didn't take enough time to really understand how it works.

It is much better in many ways , first of it does not check for visibility of every Tile there is on the board, second is it attaches/ detaches Tile only when it has to (when it appeared or disappeard due to previous iteration ) ; naive algorithm would hide all the Tiles from the previous iteration and compute the new set of all visible TIles -- and attach them .

"I would have expected a simple linear extrapolation to know all the tiles within vision"
Yeah I also extrapolate lineary the borders of visible area. Implicitly.

I've tried but there is still the bug with the minimap. However, once it is fixed, it is mergeable for me.
Well I propose to return to the old pixel based version of minimap.
Danimal's for it ... who else ?
Could you just shift that to separate issue ?

naive algorithm would hide all the Tiles from the previous iteration and compute the new set of all visible TIles -- and attach them

Yes, that's what I was expecting. Simple. To do better, your algorithm has really to be fast. I should have a deeper look as it is interesting ^^
The good thing with the current minimap is that is allows to see which tiles are culled. And from what I can tell, it is pretty good as we are not far from the tiles in vision.

Well I propose to return to the old pixel based version of minimap.

IMHO, removing a feature because it is broken by another one is not the way to go. If I implement gem tiles and break tile culling in my way, would you think disabling tile culling would be a good solution ?
Moreover, the old minimap do not work with D3D renderer. I know we only want to support OpenGL officially but as so far, D3D works out of the box, I would think it is a waste to disable it. Moreover, because it doesn't work with D3D, I would guess there is a bug / wrong use of the texture for the old minimap.

Well I propose to return to the old pixel based version of minimap.

IMHO, removing a feature because it is broken by another one is not the way to go. If I implement gem tiles and break tile culling in my way, would you think disabling tile culling would be a good solution ?

@tomluchowski We're a spare-time project, so finishing what we start is a key point if we want to stay relevant as a project. Otherwise, we'll be leaving a trail of unfinished features behind us, which is a known common cause of spare time projects death. The former OD team is plainly concerned by that sadly. But I am, too, somehow.

Btw, I consider both minimaps broken or sub-optimized, but again, we're a spare-time project. ;)

Btw, what is left to do here, apart the minimap fix?

Btw, what is left to do here, apart the minimap fix?

I would say only the minimap fix. Apart from that, the features seems to work well. I think @tomluchowski wants to go for creature culling but IMHO, it would be better to do that in a new PR

Yes, he should do that in a new PR, the poor one already bleed a lot to get this one, let's not be too cruel. ;)

"Btw, I consider both minimaps broken or sub-optimized, but again, we're a spare-time project. ;)"

sub-optimized ?
What do you mean ?
Yes I bleeded quite much , could somone give me a helping hand , or give advice how to finish this off ?

Yes I bleeded quite much , could somone give me a helping hand , or give advice how to finish this off ?

I've not looked deep enough your code to know every subtility but you said that only changed tiles are culled. That would mean that you somehow keep a list of culled tiles. If it is the case, the easiest way would be to unhide tiles before rendering the minimap (I don't know how hard it will hit performances, though). You might also want to somehow cull tiles not rendered in the minimap.

" That would mean that you somehow keep a list of culled tiles."
The secret is I never keep such a list. I could unhide the tiles, but doing that to 400x400 or 100x100 tiles at once would stall down everything. Introducing second camera is painfully difficult and beyond my head .

@OpenDungeons/developers So what's the status quo? It looks like we have an interesting new feature that also happens to break the minimap, and @tomluchowski doesn't know how to fix it. Should we merge and have someone else try to fix it, or keep it around until we find a solution?

For my part, as I've said in the comments, I couldn't easily follow the code by reading it (complex code and not enough comments). In a professional context, that would be enough to reject the PR. But this is a free time project so we don't have to be too strict. However, I'm not convinced by the use of int64_t. By experience, it is likely to hide a bug. Moreover, if @tomluchowski, the author, cannot find the list of impacted tiles from his own code, it doesn't sound very good.
From what I've seen, finding the 4 tiles based on rays might be a good idea (since rays do not seem to be too cpu consuming). But the algorithm seems too complex for such a simple need and not readable (which means hard to maintain).
And last (but not least), it breaks the minimap.
If someone wants to take the time to really understand what the code does and fix the minimap, I won't be against merging. But if I had to go for a culling algorithm, I would go for something more "naive" but I'm pretty sure at least as efficient and much more readable.
As for now, I will vote against merging.

Overall, I must say I agree with @hwoarangmy 's points which have to be fixed before going further.

But this is a free time project so we don't have to be too strict.

Here, I disagree. Please note that this very project (before the fork) was dead because of such PR were allowed, and even without any review. As a spare-time project, I'd rather say it's even more important to agree about merging PRs that are readable only. More than in a professional environment.
The area where we shouldn't be too strict, and we aren't IMHO, is about the fact to communicate within code reviews with much less rigor, and use fancy comments and jokes wherever we see fit.

But please, never undermine quality in spare-time project, you'll pay it even more than when there is someone paid to fix it.

Ok, I tend to agree with both of you. I haven't reviewed the code myself, but if you say that it hasn't the readability we strive for usually, then it's probably best kept out of the main branch.

Since the result is still relatively interesting, what do you think about merging the PR in a specific branch (i.e. not development, but a tile-culling branch that we would create)?

it's even more important to agree about merging PRs that are readable only

I would say that the most important thing is that we need to have a code repository as easy to get in as possible to allow newcomers to join.

The area where we shouldn't be too strict

I just wanted to say that it is a free time project and everybody can propose what he wants (and we cannot ask more than what one wants to do). However, it is our responsability to keep the code as clean and accessible as possible and to reject code we don't find good enough to be merged.

Since the result is still relatively interesting, what do you think about merging the PR in a specific branch

Honestly, I don't see the point. If the code is good enough, it should be merged in the main repository. If not, it should be dropped. If we keep it in a separate branch, it will rot until not mergeable and we will forget about it.

"If someone wants to take the time to really understand what the code does and fix the minimap, I won't be against merging."
I am not any algorithm God, the solution I come up with is really simple, I really tried to explain in comments what it does. It's simple interpolation based on the idea that the line formula is y = ax + b .

The problem with minimap is when I started working on culling feature it was 2D pixel based version, so my hiding or unhidding some tiles didn't broke anything. Both minimaps would cause a problem for me to work properly, the other issue being that you seem to be undecided which version should stay for good. The easiest way for me would be to go for 2d pixel verison and ask on ogre3d forum why our code does not work under DirectX, and what to do to make it working, despite me not having Windows installed...

I really put much effort and don't want to get it discarded, I wanted to have position in CV about an open- source game somehow improved by me, however clumsy I am at it tough ....

I'm closing this one as it cannot be finished for now. I've linked with corresponding issue and will close it for now.